Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PXP-11291 - Fix/data import directory notice #105

Merged
merged 26 commits into from
Apr 1, 2024

Conversation

AlbertSnows
Copy link
Contributor

@AlbertSnows AlbertSnows commented Mar 28, 2024

This PR adds a check that the directory exists, moves the original code into its own function to help with readability, and adds a test to ensure the function tries to open and write files as expected.

Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/PXP-11291

New Features

  • When running a submission order command such as data-simulator submission_order --path <path here>, the code no longer will crash with a misleading message about the .txt file if the path does not exist

Breaking Changes

Bug Fixes

Improvements

  • Minor improvement to error logging and readability when generating a submission order

Dependency updates

Deployment changes

Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@AlbertSnows AlbertSnows marked this pull request as ready for review March 28, 2024 17:45
datasimulator/main.py Outdated Show resolved Hide resolved
datasimulator/main.py Show resolved Hide resolved
datasimulator/main.py Outdated Show resolved Hide resolved
Comment on lines 148 to 149
if not path_exists:
logger.error("Cannot create file because path does not exist. Did you create a 'test-data' folder?")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think a better solution in terms of UX would be to create the directory automatically instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested that as well but @k-burt-uch is of the mind that we should just handle the error a bit more gracefully.

datasimulator/file_handling.py Outdated Show resolved Hide resolved
datasimulator/file_handling.py Outdated Show resolved Hide resolved
datasimulator/file_handling.py Outdated Show resolved Hide resolved
datasimulator/file_handling.py Outdated Show resolved Hide resolved
datasimulator/file_handling.py Outdated Show resolved Hide resolved
tests/test_file_handling.py Outdated Show resolved Hide resolved
AlbertSnows and others added 12 commits March 28, 2024 13:43
fix(improvement): generalizing error message

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>
fix(rename): better function name

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>
…' into fix/data_import_directory_notice

# Conflicts:
#	datasimulator/file_handling.py
datasimulator/main.py Outdated Show resolved Hide resolved
datasimulator/utils.py Outdated Show resolved Hide resolved
tests/test_file_handling.py Show resolved Hide resolved
tests/test_file_handling.py Outdated Show resolved Hide resolved
datasimulator/file_handling.py Outdated Show resolved Hide resolved
tests/test_file_handling.py Outdated Show resolved Hide resolved
@AlbertSnows AlbertSnows merged commit 8134901 into master Apr 1, 2024
6 checks passed
@AlbertSnows AlbertSnows deleted the fix/data_import_directory_notice branch April 1, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants